Kickstart support drop for Python < 3.7 #499

leogama · 2022-06-04T15:00:27Z

@mmckerns and @anivegesana, plus anyone interested: With the prospect of dill 0.3.6 release —it would be fun if it matched the lowest Python version supported, 3.7— in the near future, it is an opportunity to remove some cruft from the code base after the official support drop for Python 2 and Python < 3.7. Important: this is not a refactoring.

I propose to do so in discrete steps to facilitate review and minimize the chances of introducing bugs. The first steps should only remove code that Python/PyPy >= 3.7 will never run, with minimal code rewrite beyond necessary indentation changes and substitution of variables left as unconditional constants, after the other changes, by its literal values.

I plan to do each step in a separated commit, so that you guys can review the individual diffs and any unit test failure is easy to catch:

1. Remove unreachable code and unused Python version constants (e.g. IS_PYPY2), and convert the new unconditional constants left to literals.
2. Convert conditional code written as strings passed exec() due to syntax limitations to common if-else code. Remove conditional imports. Remove from __future__ imports.

Clean-up now unused types definitions if they are not in _reverse_typemap, to still allow unpickling of objects created with older combinations of python+dill versions. Unalias some types for internal usage, like BytesIO that is renamed to StringIO —it was a bit confusing the first time I met it. → moved to Support drop for Python < 3.7: step 3 — remove unused types and save_*() functions #504
Rewrite some snippets of code to take advantage of new Python 3 syntax if it improves code quality or speed, or if the used functions are deprecated in 3.7 → moved to Support drop for Python < 3.7: step 4 — update code to use Python 3.7 features #505

Neither all steps are strictly necessary, nor they need to be completed before the next release. If you have the time, it would be important to thoroughly review every change made (the more eyeballs, the better). Please feel completely free to make changes by committing directly here and to add or remove steps to the ones I'm suggesting.

Step 1 of code clean-up: mechanically remove if-else branches that would never run in Python/PyPy >= 3.7. There's still some unused or redundant code, left for the next step to facilitate review. Note: all the remaining `sys.hexversion` tests were standardized to use numerical comparison and hexadecimal integer literals (e.g. `0x30b00a7` for version 3.11.0a7) as most of them already were.

anivegesana · 2022-06-04T19:12:41Z

There are a couple more that are no longer needed. save_slice is one of them, but it will require a little bit of reasoning and trying them all out in pickle to see which ones are in CPython now.

leogama · 2022-06-05T14:12:07Z

There are a couple more that are no longer needed. save_slice is one of them, but it will require a little bit of reasoning and trying them all out in pickle to see which ones are in CPython now.

Yes, the now obsolete save_<type>() functions can be safely removed in my proposed step 3, while keeping they correspondent _create_<type>() functions for backward compatibility. And you are right in that every component removed and modified in this critical step must be evaluated carefully and tested.

mmckerns · 2022-06-08T19:43:12Z

I'd split your steps into two or three different PRs -- keep this one as (1 & 2), and create a new PR for (4). I'm not sure the best place for (3). I think we can get (1 & 2) into dill-0.3.6.

Official end of support for 3.7 from python.org is 2023-06-27. So that means dill will end support for 3.7 in 2023 or 2024. It's still a good way off.

leogama · 2022-06-09T10:06:24Z

I'd split your steps into two or three different PRs -- keep this one as (1 & 2), and create a new PR for (4). I'm not sure the best place for (3).

Agreed. Is there a way to create an empty draft PR? Or do I nead at least a stub change?

leogama · 2022-06-09T14:22:34Z

@mmckerns I managed to open the PRs with empty commits. You can add the milestone to this one now.

…ic where it's obvious

leogama · 2022-06-21T17:13:06Z

@mmckerns and @anivegesana: this is ready for review.

dill/__diff.py

tests/__main__.py

dill/detect.py

mmckerns · 2022-06-24T02:48:25Z

I've reviewed all files except _dill.py and _objects.py, and all looks good thus far. I've commented where there are/were issues. Will finish this tomorrow.

dill/_dill.py

dill/__init__.py

mmckerns

Generally LGTM. Thanks for all the hard (and tedious) work. Please address all of my comments and change requests. One thing to also watch out for in dill._dill is cases where we want to always want to trap any exception, regardless if it looks like there's only a particular type that is possible (i.e. the exception is used more like a if/else). I noted a few cases that I couldn't remember fully what scope the exception block was intended to have.

leogama · 2022-06-26T19:26:59Z

Applied the requested corrections and then merged with master HEAD. All tests passing 100%. I'm feeling this is ready to go!

leogama · 2022-06-26T19:42:42Z

Generally LGTM. Thanks for all the hard (and tedious) work.

You're welcome! 🙂 Applying one step at a time to all files sped up the process. Vim search and substitutions also helped a lot. But I'm pretty sure that reviewing for you was as much as tedious as editing for me...

One thing to also watch out for in dill._dill is cases where we want to always want to trap any exception, regardless if it looks like there's only a particular type that is possible (i.e. the exception is used more like a if/else). I noted a few cases that I couldn't remember fully what scope the exception block was intended to have.

In the end, except for the ImportErrors, I left most cases as catch-all except Exception clauses. Either because I spotted they were a general error trap or because I had no idea of the kind of error expected. However, even if it stays as it is, it would be great if you could annotate the ones you still remember the scope, no matter how vague. Sometimes it is possible to infer what kind of things can go wrong based on the context, but some generic exception blocks seem like real mysteries.

mmckerns · 2022-06-26T21:40:43Z

it would be great if you could annotate the ones you still remember the scope, no matter how vague.

Some of this code is 15 years old. I feel like I should get into a time machine and tell younger me to document the exception blocks more.

mmckerns · 2022-06-28T16:46:11Z

Applying one step at a time to all files sped up the process.

Side note: can you email me a list of the keywords you searched for in your removal of dead code? I'm making similar changes on the other UQF codes.

leogama · 2022-06-28T20:19:24Z

Applying one step at a time to all files sped up the process.

Side note: can you email me a list of the keywords you searched for in your removal of dead code? I'm making similar changes on the other UQF codes.

Not many, really. What I remember is, for the conditional code:

PY3
IS_PYPY2
OLD3?
sys.hexversion

I think you'll find most Python 2 stuff by looking for these (if the other packages use the same conventions). Perhaps look for case insensitive py3, py2 and python 2 too. But after that you can look for Py2 specific idioms:

__builtin__, __future__
try/except [ImportError] blocks for conditional imports
u".*" or u'.*' strings (just delete the u)
super(cls, self) can become super()
class .*(object): can delete (object)
exec .* statements
other things listed in https://python-future.org/compatible_idioms.html

I didn't check for everything in dill though. Also, maybe take a look at https://docs.python.org/3/howto/pyporting.html

mmckerns · 2022-06-28T23:03:14Z

Thanks, that's similar to my list. I've also included attr( and version and 0x2/0x3. I often used getattr for 2/3 compatibility.

leogama · 2022-07-01T21:55:33Z

@mmckerns: just pushed the merge commit with the trace PR.

leogama marked this pull request as ready for review June 8, 2022 17:16

This was referenced Jun 9, 2022

Support drop for Python < 3.7: step 3 — remove unused types and save_*() functions #504

Draft

Support drop for Python < 3.7: step 4 — update code to use Python 3.7 features #505

Draft

mmckerns added this to the dill-0.3.6 milestone Jun 9, 2022

Merge branch 'master' into drop-2.7-3.6 with clean-up changes

5e33b75

leogama force-pushed the drop-2.7-3.6 branch from 6079714 to 5e33b75 Compare June 19, 2022 18:01

leogama added 2 commits June 21, 2022 12:38

convert single function defined via exec() due to Python2/3 differences

2863597

drop 'from __future__ import print_function' statements

71fcc7b

leogama force-pushed the drop-2.7-3.6 branch from 2047c7d to 71fcc7b Compare June 21, 2022 15:40

leogama added 2 commits June 21, 2022 13:00

remove conditional imports due to Python 2 and old Python 3

bc87125

substitute bare 'except' clauses by 'except Exception' or more specif…

1f8db14

…ic where it's obvious

leogama force-pushed the drop-2.7-3.6 branch from 9b13c16 to 1f8db14 Compare June 21, 2022 16:34

mmckerns self-requested a review June 21, 2022 19:54